-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DNM] storage: factor out evaluation of BatchRequest #18796
Conversation
This is one of the self-contained threads to unravel for cockroachdb#18784.
This is also required for cockroachdb#18784.
See cockroachdb#18779. This doesn't carry out all the work but does all the basic required refactoring (mod cleanups). I don't love that we're passing `ReplicaEvalContext` through an interface. That should be avoidable and doing so should be instructive (I suspect we'll trim some fat in the process): Many methods roughly take the following form: ```go // GetMVCCStats returns the Replica's MVCCStats. func (rec ReplicaEvalContext) GetMVCCStats() enginepb.MVCCStats { r := rec.repl r.mu.RLock() defer r.mu.RUnlock() return *r.mu.state.Stats, nil } ``` and to define this method inside of the `batcheval` package, we'd have instead ```go type ReplicaEvalContext { // ... state struct { *sync.Mutex *storagebase.ReplicaState } } ``` though having to copy the mutex pointer smells fishy. Similar strategies can be used for most of the other methods. The downside is that `ReplicaEvalContext` will wind up being a struct with lots of pointers (i.e., perhaps a little large to sit in the hot path, though much smaller than BatchRequest!). We could accommodate that by arranging some of these pointers that never change (for example the `Engine` or the `AbortCache`) into a long-lived struct on `*Replica` instead.
Very nice! I'm not in love with the name Review status: 0 of 30 files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
Let the bike shed begin, @petermattis. I'd prefer not having to carry out a rename once I brush this up. I like |
|
I like |
I'm also ok with |
Superseded by #19888. |
The first two commits are #18785.
See #18779.
This doesn't carry out all the work but does all the basic required
refactoring (mod cleanups).
I don't love that we're passing
ReplicaEvalContext
through an interface.That should be avoidable and doing so should be instructive (I suspect
we'll trim some fat in the process):
Many methods roughly take the following form:
and to define this method inside of the
batcheval
package, we'd haveinstead